Skip to content

Conversation

@edsiper
Copy link
Member

@edsiper edsiper commented Oct 27, 2025

It aimed to fix #11068 but it wont. However is fixing some handling of the cursor

Restart the journal data cursor before enumerating the fields. Recent libsystemd releases require callers to reset the cursor between entries; otherwise the internal decompression context can be reused in an invalid state, which ultimately triggers a crash in ZSTD_freeDDict().


Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

Summary by CodeRabbit

  • Bug Fixes
    • Prevented rare crashes and incorrect log processing when reading consecutive system journal entries by resetting internal decompression state between entries, improving stability and accuracy of imports.
    • Reduced risk of runtime symbol conflicts and related crashes by making the bundled static compression library’s symbols internal, improving isolation and reliability.

@coderabbitai
Copy link

coderabbitai bot commented Oct 27, 2025

Walkthrough

Added two resets of the systemd journal data cursor/decompression state in the systemd input plugin and set static libzstd visibility to hidden in the bundled zstd CMake build.

Changes

Cohort / File(s) Change Summary
Systemd input: journal data cursor reset
plugins/in_systemd/systemd.c
Inserted two calls to sd_journal_restart_data(ctx->j) — one immediately after advancing to the next journal entry and one just before enumerating entry fields — to reset the journal data/decompression state before processing entry data.
Bundled zstd: static library symbol visibility
lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt
Added set_target_properties(libzstd_static PROPERTIES C_VISIBILITY_PRESET hidden VISIBILITY_INLINES_HIDDEN YES) right after add_library(libzstd_static STATIC ...) to hide exported symbols from the static libzstd build.

Sequence Diagram(s)

sequenceDiagram
    participant Reader as in_systemd_collect
    participant Journal as sd_journal

    Note over Reader,Journal #EFEFEF: Per-entry processing (new calls highlighted)
    Reader->>Journal: sd_journal_next() / advance to next entry
    Reader->>Journal: sd_journal_restart_data()    -- reset decompression/cursor
    Reader->>Journal: sd_journal_enumerate_data()  -- enumerate fields
    Reader->>Journal: sd_journal_restart_data()    -- ensure fresh enumeration state
    alt field data present
        Journal-->>Reader: field data items
    else no data / end
        Journal-->>Reader: no more fields
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Heterogeneous changes: one small runtime change in plugin logic, one build-system change affecting symbol visibility and linking.
  • Pay attention to:
    • Correct placement and potential redundant calls of sd_journal_restart_data in plugins/in_systemd/systemd.c.
    • Build implications of hiding symbols in libzstd_static (linking, symbol collisions, consumers that expected visible inlines).
    • Reproduce/test on affected platforms (RHEL10/systemd versions) to validate crash regression.

Poem

🐰 I hop in code with careful paws,
I nudge the journal, mend its flaws.
A hidden coat for zstd's song,
Two tiny taps to keep logs strong.
Happy hops — the system hums along.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The PR includes changes to two files: plugins/in_systemd/systemd.c (cursor reset calls) and lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt (symbol visibility configuration). While the systemd.c changes are clearly aligned with the PR title and objective, the zstd CMakeLists.txt change adding visibility presets is not mentioned in the PR description, which states only that "the patch restarts the systemd journal data cursor before enumerating fields." This zstd build configuration change appears disconnected from the stated objective of cursor reset and lacks clear justification for inclusion in this PR. Either remove the zstd CMakeLists.txt change if it is not necessary for fixing the cursor reset issue, or provide clear documentation explaining how the symbol visibility control is an integral part of this fix and why both changes must be merged together rather than separately.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Linked Issues Check ❓ Inconclusive The PR addresses one key aspect of issue #11068 by implementing defensive checks around sd_journal_enumerate_data handling through cursor reset calls. However, issue #11068 requires comprehensive fixes including chunk integrity validation on load, detection and handling of irrecoverable chunks, comprehensive zstd dict/dctx lifecycle management, and ensuring storage.delete_irrecoverable_chunks semantics are honored. The changes in this PR implement only the cursor reset defensive check and appear to be a partial fix targeting one specific cause of the crash rather than the full set of requirements outlined in the linked issue. Clarify whether this PR is intended as a complete fix for issue #11068 or as a partial fix addressing only the cursor reset aspect. If it is a partial fix, document which requirements are addressed and which are deferred to other PRs. If it is intended to be complete, provide evidence that chunk validation, irrecoverable chunk handling, and zstd lifecycle management are also addressed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "in_systemd: always reset cursor before iterating fields" is concise, clear, and directly summarizes the main change described in the summary: adding two calls to sd_journal_restart_data(ctx->j) to reset the decompression state before processing entry data and before enumerating fields. The title accurately reflects the primary objective of the patch without being overly vague or misleading.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch in_systemd-reset-cursor

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@edsiper
Copy link
Member Author

edsiper commented Oct 28, 2025

Added another update to avoid zstd exporting symbols...

@edsiper
Copy link
Member Author

edsiper commented Nov 10, 2025

note: CI error is with CI Fuzz with credentials (already fixed in master)

@edsiper edsiper force-pushed the in_systemd-reset-cursor branch from 6157556 to 2a64814 Compare November 10, 2025 04:00
@edsiper edsiper requested a review from cosmo0920 as a code owner November 10, 2025 04:00
@edsiper
Copy link
Member Author

edsiper commented Nov 10, 2025

Removed the zstd commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fluent-bit crashes with a coredump when running on RHEL10

2 participants